Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial API proposal (for merging proposed YAML of PR#9) #10

Merged
merged 16 commits into from
Nov 12, 2024

Conversation

yamamoto0104
Copy link
Contributor

What type of PR is this?

  • enhancement/feature

What this PR does / why we need it:

This is the initial proposal from KDDI for the Number Recycling API

Which issue(s) this PR fixes:

Fixes #8

Special notes for reviewers:

This PR is for merging proposed YAML of PR #9.

Changelog input

 release-note

Additional documentation

None

docs

@yamamoto0104
Copy link
Contributor Author

Hello @eric-murray , @grgpapadopoulos
I changed the format of specifiedDate parameter from RFC 3339[YYYY-MM-dd 'T'HH:mm:ss.SSSZ] to ISO 8601((YYYY-MM-DD). Because we are not considering use cases that specify time.

@eric-murray
Copy link
Contributor

I changed the format of specifiedDate parameter from RFC 3339[YYYY-MM-dd 'T'HH:mm:ss.SSSZ] to ISO 8601((YYYY-MM-DD). Because we are not considering use cases that specify time.

So, a number of comments here:

  • Format "YYYY-MM-DD" is both a valid RFC 3339 and ISO 8601 format, so there is no reason to reference ISO 8601 rather than RFC 3339. The argument against referencing ISO standards is that obtaining the standard costs money. If it is not expected that the API consumer is required to consult an ISO standard, then don't reference it.
  • Making specifiedDate just a string makes parsing harder for API implementations. At the very least, add a pattern to simplify validation. But a simple pattern such as \d\d\d\d-\d\d-\d\d would allow invalid dates such as 2024-00-00, and so you end up requiring complex patterns and/or custom validation algorithms.
  • Far better just to make specifiedDate require format: date-time. Code generators (both for client and server implementations) know all about date-time, so generation and validation of the string becomes automatic. It is irrelevant that this allows a time to be specified as well as a date, as the time can be ignored if the API consumer specifies that.

So my recommendation is to make specifiedDate follow format: date-time, requiring the API consumer to follow RFC 3339. This also complies with the Commonalities guidelines for data types.

@yamamoto0104
Copy link
Contributor Author

Hello @eric-murray
Thanks for your comment. We updated the YAML to use either specifiedDateTime or specifiedDate parameter.
oneOf:
- required: [specifiedDateTime]
- required: [specifiedDate]

I understand format: date-time is described on Commonalities guidelines. However, we are not considering use cases that specify time for this API. Format "YYYY-MM-DD" is also used for KYC API. Thus, we'd like to choose the data related parameter based on the use case.

@eric-murray
Copy link
Contributor

@yamamoto0104
This is over-complicating things. If want to only accept dates, then just specify format: date. See here.

There really is no difference between an ISO 8601 formatted date and an RFC 3339 formatted date.

@yamamoto0104
Copy link
Contributor Author

Hello @eric-murray , @grgpapadopoulos
I'll prepare to propose an additional content to description. So please wait for your approval until I propose this.

@eric-murray
Copy link
Contributor

@yamamoto0104
OK. I'm happy with the current proposal, so will approve after you update the field description.

In case you have not seen it, there is an useful online comparison of valid RFC 3339 and ISO 8601 date/time formats here.

@yamamoto0104
Copy link
Contributor Author

Hello @eric-murray and @grgpapadopoulos
I update the description as follow:

  1. True and false scenarios for API response were added.
  2. The figures for True/Faise were added to /documentation/API_documentation/assets/ folder. If this PR is merged, the URL of the figures in the YAML file will be updated.

@eric-murray
Copy link
Contributor

If this PR is merged, the URL of the figures in the YAML file will be updated.

We know in advance what the links will be, so can specify them now. I updated them both as suggested changes so you can see how links to your repository will be mapped to links in the CAMARA repository.

@Masa8106
Copy link
Contributor

Masa8106 commented Nov 5, 2024

If this PR is merged, the URL of the figures in the YAML file will be updated.

We know in advance what the links will be, so can specify them now. I updated them both as suggested changes so you can see how links to your repository will be mapped to links in the CAMARA repository.

Thank you, @eric-murray.

You are right that we already know the links in CAMARA Repository.

On the other hand, as you can find in the last meeting minutes, I have asked all members to review the initial proposal (I assume) by the next meeting. My preference is to keep it as it is by the next meeting for reviewers. Please let us update it with your suggested changes on the links after the next meeting, and then merge it to the CAMARA repository.

@eric-murray
Copy link
Contributor

@Masa8106
OK, I'll give my approval now. But note that, when you update the links, it will cancel existing approvals and you will need the PR to be re-approved before it can be merged.

eric-murray
eric-murray previously approved these changes Nov 5, 2024
@Masa8106
Copy link
Contributor

Masa8106 commented Nov 5, 2024

@eric-murray
Thanks for your understanding. I see your note. Once we update the links, we will do re-approval.

@Masa8106 Masa8106 requested a review from eric-murray November 12, 2024 10:13
@Masa8106
Copy link
Contributor

Hello @eric-murray and @grgpapadopoulos
The links were updated.
During the meeting today, there was no additional comment.
Would you approve it again? Once either of you approve, I will merge this PR. Thank you.

@Masa8106 Masa8106 merged commit d413ce7 into camaraproject:main Nov 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial proposal for Number Recycling API definition
3 participants